86b7gfrw5 - fix: pagination on sponsor forms when rows per page change#758
86b7gfrw5 - fix: pagination on sponsor forms when rows per page change#758
Conversation
ghost
assigned
📝 WalkthroughWalkthroughRenames getSponsorForms' second parameter from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/actions/sponsor-forms-actions.js (1)
117-132: CapcurrentPagetolastPagewhen perPage changes.Line 131 only resets when
perPage > totalCount. IfperPageincreases but still <=totalCount,currentPagecan still exceed the last page (e.g., totalCount=50, perPage=25, currentPage=3). This will keep sending an invalid page and can still trigger the backend error you’re trying to avoid. Consider computinglastPageand cappingcurrentPage.🐛 Proposed fix
- // Resets page to avoid backend error. - const page = perPage > totalCount ? 1 : currentPage; + // Reset/cap page to avoid backend errors. + const safeTotal = Number.isFinite(totalCount) ? totalCount : 0; + const lastPage = Math.max(1, Math.ceil(safeTotal / perPage)); + const page = Math.min(currentPage, lastPage);
🧹 Nitpick comments (1)
src/actions/__tests__/sponsor-forms-actions.test.js (1)
62-121: Add a test forcurrentPage > lastPagewhen perPage changes.Right now the tests don’t cover the scenario where
perPagechanges butcurrentPageis now beyond the last page (even ifperPage <= totalCount). Adding a test will lock in the intended cap behavior once the fix above is applied.✅ Suggested test case
+ it("should cap page to last page when currentPage exceeds last page after perPage change", async () => { + const store = mockStore({ + currentSummitState: { currentSummit: {} }, + sponsorFormsListState: { totalCount: 50 } + }); + + store.dispatch(getSponsorForms("", 5, 25, "id", 1, false, [])); + await flushPromises(); + + expect(getRequest).toHaveBeenCalledWith( + expect.anything(), + expect.anything(), + expect.anything(), + expect.anything(), + { + hideArchived: false, + order: "id", + orderDir: 1, + page: 2, + perPage: 25, + term: "" + } + ); + });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/actions/__tests__/sponsor-forms-actions.test.jssrc/actions/sponsor-forms-actions.js
🧰 Additional context used
🧬 Code graph analysis (2)
src/actions/sponsor-forms-actions.js (4)
src/reducers/sponsors/sponsor-form-items-list-reducer.js (1)
currentPage(79-83)src/reducers/sponsors/sponsor-forms-list-reducer.js (3)
currentPage(95-99)currentPage(190-194)currentPage(222-227)src/reducers/sponsors/sponsor-page-forms-list-reducer.js (2)
currentPage(96-100)currentPage(138-142)src/utils/constants.js (6)
DEFAULT_CURRENT_PAGE(73-73)DEFAULT_CURRENT_PAGE(73-73)DEFAULT_PER_PAGE(75-75)DEFAULT_PER_PAGE(75-75)DEFAULT_ORDER_DIR(91-91)DEFAULT_ORDER_DIR(91-91)
src/actions/__tests__/sponsor-forms-actions.test.js (1)
src/actions/sponsor-forms-actions.js (2)
getSponsorForms(106-168)getSponsorForms(106-168)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
ghost
force-pushed
the
fix/pagination-does-no-work
branch
from
72bcbd9 to
168246c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/actions/__tests__/sponsor-forms-actions.test.js`:
- Around line 46-53: The mock dispatch block incorrectly falls through and
dispatches receiveActionCreator twice when it is a function; update the block
that checks typeof receiveActionCreator === "function" (the anonymous function
passed to new Promise) to prevent fallthrough by returning after calling
dispatch(receiveActionCreator({ response: {} })) and resolve({ response: {} })
(or change to an if/else) so the unconditional dispatch(receiveActionCreator)
and resolve(...) lines are not executed in that branch.
🧹 Nitpick comments (1)
src/actions/__tests__/sponsor-forms-actions.test.js (1)
62-121: Good test coverage for the main scenarios.The two tests effectively validate the core pagination fix behavior. Consider adding edge case tests for completeness:
totalCount === 0(empty list)perPage === totalCount(boundary condition)- Initial state where
totalCountmight be undefined
| return new Promise((resolve) => { | ||
| if (typeof receiveActionCreator === "function") { | ||
| dispatch(receiveActionCreator({ response: {} })); | ||
| resolve({ response: {} }); | ||
| } | ||
| dispatch(receiveActionCreator); | ||
| resolve({ response: {} }); | ||
| }); |
There was a problem hiding this comment.
Bug in mock: receiveActionCreator is dispatched twice when it's a function.
When receiveActionCreator is a function, the code dispatches it at line 48 and resolves, but then falls through to line 51-52 which dispatches it again unconditionally. The resolve at line 49 doesn't exit the function.
🐛 Proposed fix
return new Promise((resolve) => {
if (typeof receiveActionCreator === "function") {
dispatch(receiveActionCreator({ response: {} }));
- resolve({ response: {} });
+ return resolve({ response: {} });
}
dispatch(receiveActionCreator);
resolve({ response: {} });
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return new Promise((resolve) => { | |
| if (typeof receiveActionCreator === "function") { | |
| dispatch(receiveActionCreator({ response: {} })); | |
| resolve({ response: {} }); | |
| } | |
| dispatch(receiveActionCreator); | |
| resolve({ response: {} }); | |
| }); | |
| return new Promise((resolve) => { | |
| if (typeof receiveActionCreator === "function") { | |
| dispatch(receiveActionCreator({ response: {} })); | |
| return resolve({ response: {} }); | |
| } | |
| dispatch(receiveActionCreator); | |
| resolve({ response: {} }); | |
| }); |
🤖 Prompt for AI Agents
In `@src/actions/__tests__/sponsor-forms-actions.test.js` around lines 46 - 53,
The mock dispatch block incorrectly falls through and dispatches
receiveActionCreator twice when it is a function; update the block that checks
typeof receiveActionCreator === "function" (the anonymous function passed to new
Promise) to prevent fallthrough by returning after calling
dispatch(receiveActionCreator({ response: {} })) and resolve({ response: {} })
(or change to an if/else) so the unconditional dispatch(receiveActionCreator)
and resolve(...) lines are not executed in that branch.
|
Branch rebased |
src/actions/sponsor-forms-actions.js
Outdated
| ) => | ||
| async (dispatch, getState) => { | ||
| const { currentSummitState } = getState(); | ||
| const { currentSummitState, sponsorFormsListState } = getState(); |
There was a problem hiding this comment.
@niko-exo we need to review similar code before proposing a solution
please do check here
ghost
force-pushed
the
fix/pagination-does-no-work
branch
from
168246c to
cdd3d13
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/actions/sponsor-forms-actions.js (1)
106-161:⚠️ Potential issue | 🔴 CriticalFix meta payload field name: action sends
currentPagebut reducer expectspage.Line 160 sends
currentPagein the action meta, but the reducer (sponsor-forms-list-reducer.js line 81) destructurespage. This causespageto beundefined, breaking pagination state. Change the meta payload to usepageinstead ofcurrentPageto match the reducer's expectation, or update the reducer to destructurecurrentPage.
|
@smarcet Changes implemented and manual tested. |
1 participant
ref: https://app.clickup.com/t/86b7gfrw5
86b7gfrw5 - fix: pagination on sponsor forms when rows per page change
Changelog
Links
86b7gfrw5 - Pagination does not function, throws error
Evidence
2026-01-16_15-09-29.-.evidence.-.86b7gfrw5.mp4
Summary by CodeRabbit
Tests
Bug Fixes